better TUI perf & better result everywhere#179
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const filteredCommands = useMemo(() => { | ||
| if (!input) return COMMANDS; | ||
| return filterCommands(trimmedInput); | ||
| }); | ||
| }, [input]); |
There was a problem hiding this comment.
Out-of-bounds selection index
selectedIndex is never reset/clamped when filteredCommands changes, but tab/return read filteredCommands[selectedIndex] (lines 40-55). If a user navigates down, then types more characters so the filtered list shrinks, selectedIndex can point past the end and selection becomes a no-op. Consider clamping/resetting selectedIndex whenever filteredCommands.length changes.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/cli/src/tui/components/command-palette.tsx
Line: 20:23
Comment:
**Out-of-bounds selection index**
`selectedIndex` is never reset/clamped when `filteredCommands` changes, but `tab`/`return` read `filteredCommands[selectedIndex]` (lines 40-55). If a user navigates down, then types more characters so the filtered list shrinks, `selectedIndex` can point past the end and selection becomes a no-op. Consider clamping/resetting `selectedIndex` whenever `filteredCommands.length` changes.
How can I resolve this? If you propose a fix, please make it concise.| const selectRepo = () => { | ||
| const selectedRepo = filteredRepos()[selectedIndex()]; | ||
| if (selectedRepo) { | ||
| const idx = curInputIdx(); | ||
| const currentState = props.inputState; | ||
| const newContent = '@' + selectedRepo.name + ' '; | ||
| const newState = [ | ||
| ...currentState.slice(0, idx), | ||
| { content: newContent, type: 'mention' as const }, | ||
| ...currentState.slice(idx + 1) | ||
| ]; | ||
| props.setInputState(newState); | ||
| const inputRef = props.inputRef; | ||
| if (inputRef) { | ||
| let newCursorPos = 0; | ||
| for (let i = 0; i <= idx; i++) { | ||
| newCursorPos += i === idx ? newContent.length : getDisplayLength(currentState[i]!); | ||
| } | ||
| // Calculate the new text and update textarea | ||
| const newText = newState | ||
| .map((p) => (p.type === 'pasted' ? `[~${p.lines} lines]` : p.content)) | ||
| .join(''); | ||
| inputRef.setText(newText); | ||
| inputRef.editBuffer.setCursor(0, newCursorPos); | ||
| } | ||
| const selectedRepo = filteredRepos[selectedIndex]; | ||
| if (!selectedRepo) return; | ||
|
|
There was a problem hiding this comment.
Selection can become invalid
selectRepo() reads filteredRepos[selectedIndex], but selectedIndex is never clamped/reset when filteredRepos changes. If the user moves the selection and then types to narrow results, selectedIndex can go out of range and selectRepo() will early-return, making Tab/Enter appear broken.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/cli/src/tui/components/repo-mention-palette.tsx
Line: 63:66
Comment:
**Selection can become invalid**
`selectRepo()` reads `filteredRepos[selectedIndex]`, but `selectedIndex` is never clamped/reset when `filteredRepos` changes. If the user moves the selection and then types to narrow results, `selectedIndex` can go out of range and `selectRepo()` will early-return, making Tab/Enter appear broken.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Cursor position units mismatch
InputSection treats cursorPosition as a linear offset across the full composed input, but MainInput sets it to e.visualColumn in onCursorChange (line 328), which is per-line. Other code paths set it to logicalCursor.col orrow * availableWidth + col(paste path at lines 136-138). Once the input wraps to multiple lines, mention/command detection and palettes that depend oncursorPosition` will behave incorrectly unless a single consistent coordinate system is used.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/cli/src/tui/components/main-input.tsx
Line: 327:329
Comment:
**Cursor position units mismatch**
`InputSection` treats `cursorPosition` as a linear offset across the full composed input, but `MainInput` sets it to `e.visualColumn` in `onCursorChange` (line 328), which is per-line. Other code paths set it to `logicalCursor.col` orrow * availableWidth + col` (paste path at lines 136-138). Once the input wraps to multiple lines, mention/command detection and palettes that depend on `cursorPosition` will behave incorrectly unless a single consistent coordinate system is used.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Stale filtered repo list
filteredRepos is memoized but its computation calls curInputIdx() which reads props.inputRef.logicalCursor (lines 25-41). The memo deps include props.cursorPosition, but curInputIdx() doesn't use it; if the cursor moves without changing inputState or the inputRef identity, filteredRepos can stay stale and show the wrong suggestions.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/cli/src/tui/components/repo-mention-palette.tsx
Line: 44:50
Comment:
**Stale filtered repo list**
`filteredRepos` is memoized but its computation calls `curInputIdx()` which reads `props.inputRef.logicalCursor` (lines 25-41). The memo deps include `props.cursorPosition`, but `curInputIdx()` doesn't use it; if the cursor moves without changing `inputState` or the `inputRef` identity, `filteredRepos` can stay stale and show the wrong suggestions.
How can I resolve this? If you propose a fix, please make it concise.
bmdavis419
force-pushed
the
davis/react-tui-experiment
branch
from
58cf892 to
f34426a
Compare
bmdavis419
changed the title
| if (!inputRef) return; | ||
| inputRef.gotoBufferEnd(); | ||
| const cursor = inputRef.logicalCursor; | ||
| setCursorPosition(cursor.col); |
There was a problem hiding this comment.
Cursor position calculation inconsistency breaks multi-line input history navigation. Uses cursor.col but should use cursor.row * availableWidth + cursor.col to match main-input.tsx:48,255. When navigating to history entries that wrap, cursor position will be wrong.
| setCursorPosition(cursor.col); | |
| const availableWidth = Math.max(1, terminalDimensions.width - 4); | |
| setCursorPosition(cursor.row * availableWidth + cursor.col); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/cli/src/tui/components/input-section.tsx
Line: 193:193
Comment:
Cursor position calculation inconsistency breaks multi-line input history navigation. Uses `cursor.col` but should use `cursor.row * availableWidth + cursor.col` to match main-input.tsx:48,255. When navigating to history entries that wrap, cursor position will be wrong.
```suggestion
const availableWidth = Math.max(1, terminalDimensions.width - 4);
setCursorPosition(cursor.row * availableWidth + cursor.col);
```
How can I resolve this? If you propose a fix, please make it concise.
Merge activity
|
1 participant
Greptile Overview
Greptile Summary
This PR migrates the CLI TUI from Solid.js to React and adopts the better-result pattern across web APIs.
Major Changes:
@opentui/solidto@opentui/reactwith React 19focus-registry.tsfor global focus management andopentui-hooks.tsfor React event wrappersCritical Issue:
input-section.tsx:193has cursor position calculation bug that will break history navigation on multi-line wrapped inputPlan Files:
.codex/commands/adopt-better-result.md- Command documentation for better-result adoption.codex/skills/better-result-adopt/SKILL.md- Skill definition for migration workflowapps/web/better-result-audit.md- Tracks which modules have adopted the patternThe better-result refactoring follows the documented pattern correctly, maintaining boundary-safe error handling while preserving backward compatibility.
Confidence Score: 3/5
Important Files Changed
Last reviewed commit: cd12e24